Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: courier template configs #2156

Merged
merged 37 commits into from
Feb 8, 2022
Merged

Conversation

Benehiko
Copy link
Contributor

@Benehiko Benehiko commented Jan 18, 2022

Related issue(s)

Implements: #2054

Checklist

  • I have read the contributing guidelines.
  • I have referenced an issue containing the design document if my change
    introduces a new feature.
  • I am following the
    contributing code guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security. vulnerability, I
    confirm that I got green light (please contact
    [email protected]) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

Further Comments

@codecov
Copy link

codecov bot commented Jan 18, 2022

Codecov Report

Merging #2156 (6f766f5) into v0.8 (03b687f) will increase coverage by 0.14%.
The diff coverage is 89.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##             v0.8    #2156      +/-   ##
==========================================
+ Coverage   75.70%   75.84%   +0.14%     
==========================================
  Files         295      296       +1     
  Lines       15715    15858     +143     
==========================================
+ Hits        11897    12028     +131     
- Misses       2963     2971       +8     
- Partials      855      859       +4     
Impacted Files Coverage Δ
selfservice/strategy/link/sender.go 71.27% <50.00%> (ø)
driver/config/config.go 81.89% <71.42%> (-0.54%) ⬇️
courier/template/load_template.go 74.50% <82.00%> (+7.84%) ⬆️
courier/template/testhelpers/testhelpers.go 95.06% <95.06%> (ø)
courier/courier.go 69.44% <100.00%> (ø)
courier/template/recovery_invalid.go 100.00% <100.00%> (ø)
courier/template/recovery_valid.go 100.00% <100.00%> (ø)
courier/template/stub.go 100.00% <100.00%> (ø)
courier/template/verification_invalid.go 100.00% <100.00%> (ø)
courier/template/verification_valid.go 100.00% <100.00%> (ø)
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 03b687f...6f766f5. Read the comment docs.

@Benehiko Benehiko marked this pull request as ready for review January 27, 2022 10:59
Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! This looks really solid. A few follow ups:

  • Could you please add documentation to the courier section which explains how to customize individual templates?
  • Is it possible to load only one template from a custom source, and then all other templates are used from the default source?
  • Could you customize the recovery or verification e2e test and set one custom template and then, in the respective e2e test, ensure that the custom template is being used? You can add one or two custom templates to here and then assert the contents around here. Alternatively you could also update the config on demand in the e2e test and write an individual test just for the custom template as e2e. To set a custom config value within your test, see here and subsuquently here. I recommend using base64 based template loading to make your life easier.
  • Can you please add a schema test to verify that your JSON Schema is correct?

courier/template/load_template.go Outdated Show resolved Hide resolved
courier/template/load_template_test.go Show resolved Hide resolved
courier/template/verification_invalid_test.go Show resolved Hide resolved
driver/config/config.go Outdated Show resolved Hide resolved
@Benehiko Benehiko changed the base branch from master to v0.8 January 31, 2022 15:05
@aeneasr
Copy link
Member

aeneasr commented Feb 1, 2022

@Benehiko are you waiting for a review on this one?

@Benehiko
Copy link
Contributor Author

Benehiko commented Feb 1, 2022

I'm still writing e2e tests, it's almost ready

@Benehiko Benehiko mentioned this pull request Feb 4, 2022
7 tasks
@Benehiko Benehiko requested a review from aeneasr February 4, 2022 16:27
@Benehiko
Copy link
Contributor Author

Benehiko commented Feb 4, 2022

Should be good to go on green CI

README.md Outdated Show resolved Hide resolved
test/e2e/run.sh Show resolved Hide resolved
embedx/config.schema.json Show resolved Hide resolved
courier/template/load_template.go Outdated Show resolved Hide resolved
courier/template/load_template_test.go Show resolved Hide resolved
@Benehiko Benehiko requested a review from aeneasr February 8, 2022 13:07
@aeneasr aeneasr merged commit e9087a1 into ory:v0.8 Feb 8, 2022
@Benehiko Benehiko deleted the courier-templates-configs branch February 8, 2022 15:44
aeneasr pushed a commit that referenced this pull request Feb 11, 2022
It is now possible to override individual courier email templates using the configuration system!

Closes #2054
aeneasr pushed a commit that referenced this pull request Feb 14, 2022
It is now possible to override individual courier email templates using the configuration system!

Closes #2054
@vinckr vinckr mentioned this pull request Mar 18, 2022
peturgeorgievv pushed a commit to senteca/kratos-fork that referenced this pull request Jun 30, 2023
It is now possible to override individual courier email templates using the configuration system!

Closes ory#2054
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants